Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Fix TAC opening with keyboard #12285

Merged
merged 10 commits into from
Feb 29, 2024
Merged

Fix TAC opening with keyboard #12285

merged 10 commits into from
Feb 29, 2024

Conversation

florianduros
Copy link
Contributor

@florianduros florianduros commented Feb 26, 2024 •
edited
Loading

Checklist

  • Tests written for new code (and old code if feasible)
  • Linter and other CI checks pass
  • Sign-off given on the changes (see CONTRIBUTING.md)

Closes element-hq/element-web#27016

The compound Menu component is putting the keyboard listeners on the trigger through the props but the AccessibilityButton is already handling some keyboard events.

  • Replace the current implementation with the tooltip and icon button of compound components.
  • The compound tooltip visual is different than the EW Web
Enregistrement.de.l.ecran.2024-02-28.a.16.54.25.mov

@florianduros florianduros added the T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems label Feb 26, 2024
@florianduros florianduros force-pushed the florianduros/tac/space-open branch from d3bd43d to 5a1e5d0 Compare February 26, 2024 14:53
@florianduros florianduros marked this pull request as ready for review February 26, 2024 15:21
@florianduros florianduros requested a review from a team as a code owner February 26, 2024 15:21
t3chguy
t3chguy suggested changes Feb 26, 2024
View reviewed changes
Copy link
Member

@t3chguy t3chguy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do we get from using AccessibilityButton without its accessible keyboard handling? Why not just switch to Compound button for usecases which need disableKeyboardOverrides

@florianduros
Copy link
Contributor Author

What do we get from using AccessibilityButton without its accessible keyboard handling? Why not just switch to Compound button for usecases which need disableKeyboardOverrides

Because, it means add all the glue to between the EW tooltip and the compound button.
The compound tooltip doesn't have the same style than the other tooltip in the spaces, so I'm using the old one of EW

@t3chguy
Copy link
Member

t3chguy commented Feb 26, 2024

We have a 50/50-ish mix of Compound/local tooltips already, so please do not consider that a blocker, preferring Compound tooltips going forward

@florianduros
Copy link
Contributor Author

florianduros commented Feb 26, 2024 •
edited
Loading

We have a 50/50-ish mix of Compound/local tooltips already, so please do not consider that a blocker, preferring Compound tooltips going forward

Okay, I'll discard the use of the AccessibilityButton here and use the Tooltip and IconButton of compound instead

@t3chguy
Copy link
Member

t3chguy commented Feb 26, 2024

We have a 50/50-ish mix of Compound/local tooltips already, so please do not consider that a blocker, preferring Compound tooltips going forward

Okay, I'll discard the use of the AccessibilityButton here and use the Tooltip and IconButton of compound instead

Compund ftw

@florianduros florianduros force-pushed the florianduros/tac/space-open branch from 2f2295f to 0387697 Compare February 27, 2024 15:39
@florianduros florianduros force-pushed the florianduros/tac/space-open branch from 59cee43 to fbee2ad Compare February 28, 2024 13:13
@florianduros florianduros force-pushed the florianduros/tac/space-open branch from c6e0662 to 1b840d7 Compare February 28, 2024 16:10
@florianduros florianduros requested a review from t3chguy February 28, 2024 17:00
@florianduros florianduros added this pull request to the merge queue Feb 29, 2024
Merged via the queue into develop with commit 29b79ef Feb 29, 2024
22 checks passed
@florianduros florianduros deleted the florianduros/tac/space-open branch February 29, 2024 14:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@t3chguy t3chguy t3chguy approved these changes

@MidhunSureshR MidhunSureshR Awaiting requested review from MidhunSureshR MidhunSureshR is a code owner automatically assigned from matrix-org/element-web-reviewers

Assignees
No one assigned
Labels
T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Threads activity center can't be opened via keyboard
2 participants
@florianduros @t3chguy